fix: preserve null semantics in expression simplification - #24258
fix: preserve null semantics in expression simplification#24258fzlzjerry wants to merge 5 commits into
Conversation
github-actions
Bot
added
optimizer
|
I'll try and review it later today |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24258 +/- ##
========================================
Coverage 81.01% 81.01%
========================================
Files 1106 1106
Lines 383974 384228 +254
Branches 383974 384228 +254
========================================
+ Hits 311085 311297 +212
- Misses 54554 54577 +23
- Partials 18335 18354 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@neilconway thanks for the heads up, I'll try and do a pass on all three tonight/tomorrow morning (UK time). |
|
Thanks for flagging these. I compared the scopes:
I'll keep the current comparison available for the review pass, then promptly rebase and drop whichever overlapping implementation the maintainers decide not to use. |
|
I think the logical plans that this creates seem worse in some of these trivial cases, which ideally will keep optimizing work away while maintaining correctness in more complex cases. I'm not sure if its because of the default nullability of stuff coming out the parser or something else. |
|
Addressed both plan concerns in c2bdac8 and synced the branch with the latest The general expression simplification still preserves NULL in projection contexts, while The overlapping |
Which issue does this PR close?
Rationale for this change
Set algebra over
IN/NOT INexpressions can erase SQL three-valued-logic semantics when either the tested expression or a list item is nullable. Thearray_hastoINrewrite has a similar mismatch: an array containing NULL does not behave like anINlist containing NULL.Preserving those NULL branches is required in general expression contexts. In a filter, however, both FALSE and NULL reject the row, so the optimizer can still produce the strongest equivalent plan there.
What changes are included in this PR?
IN/NOT INset operations unchanged when list items may be NULL, and preserve the NULL branch when an otherwise safe set operation becomes a constant for a nullable tested expression.array_hastoINwhen a nullable array element would change the result, while retaining the safe non-volatile same-expression case.EliminateFilterto simplify positiveAND/ORtrees according to filter semantics. This restores plans such asEmptyRelationfor disjointINpredicates while retaining correct NULL behavior in projections. The simplification deliberately does not crossNOTor arbitrary expressions.Are these changes tested?
Yes. The following checks pass:
cargo clippy --all-targets --all-features -- -D warnings./dev/rust_lint.shcargo test --profile=ci --test sqllogictestsprlimit --nofile=65536:65536 -- env RUST_BACKTRACE=1 cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-cli --workspace --lib --tests --bins --features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryptionAre there any user-facing changes?
Yes. Expressions involving nullable inputs retain their SQL three-valued-logic results, while equivalent filter predicates continue to optimize away rejected rows. There are no public API changes.